feat(parser): use str for SES EmailStr when email_validator package is omitted #1259
Conversation
pull-request-size
bot
added
the
size/S
|
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
|
This is my first PR on a Open Source tool, If guidance is available I'm open to take what's necessary to make this PR cleaner, and compliant. |
mgochoa
force-pushed
the
fix/998-emai-validator
branch
from
8a9590d to
c0972cc
Compare
|
Hey @mgochoa thanks a lot for taking the time to add this guard. More importantly, your first PR 🎉 !
This is actually the intended behaviour because when installing That being said, I can see this will continue to happen so best to add this small fix before we tackle it long-term. I'm making two small adjustments and we should be able to merge :) |
heitorlessa
changed the title
| if has_email_validator: | ||
| from pydantic.networks import EmailStr | ||
| else: | ||
| logging.warning("email_validator package is not installed") |
There was a problem hiding this comment.
Initialize a logger and change it to debug, so we'll know where it comes from.
logger = logging.getLogger(__name__)
...
logger.debug("email_validator package is not installed; using str instead")
| from pydantic.networks import EmailStr | ||
| else: | ||
| logging.warning("email_validator package is not installed") | ||
| EmailStr = NewType("EmailStr", str) # type: ignore[no-redef, misc] |
There was a problem hiding this comment.
NewType has recently been converted to a class which means it'll have a runtime impact. Since we're looking at a string only IF someone didn't follow our recommended install, use a Type Alias instead.
You can also bring this up to save a else branch, for example:
from pydantic import BaseModel
EmailStr = str
if has_email_validator:
from pydantic import EmailStr
class Email(BaseModel):
address: EmailStr
feedback_channel = Email("[email protected]")
print(feedback_channel.address)| from pydantic.types import PositiveInt | ||
|
|
||
| from ..types import Literal | ||
|
|
||
| has_email_validator = "email_validator" in sys.modules |
There was a problem hiding this comment.
I suspect this will always be False because Pydantic is loading it dynamically, and this will only be true afterwards. You'll need to check if the module is actually installed instead of loaded - I can't recall by heart a 3.6+ compatible way in importlib/pkg_resources to do that. I can help look for that after I finish another doc PR this week
| from pydantic.types import PositiveInt | ||
|
|
||
| from ..types import Literal | ||
|
|
||
| has_email_validator = "email_validator" in sys.modules | ||
|
|
||
| if has_email_validator: |
There was a problem hiding this comment.
we're gonna need #pragma: nocover too because this will decrease our tests despite being intended behaviour ;-)
heitorlessa
force-pushed
the
develop
branch
3 times, most recently
from
42c2896 to
06965bb
Compare
heitorlessa
force-pushed
the
develop
branch
2 times, most recently
from
742ccf5 to
b582992
Compare
|
Hi @mgochoa thank you again so much for this PR! Any change you could incorporate the review feedback so we can merge this? |
Issue number: #998
Summary
When
email_validatorpackage is not present in the dependencies,pydantic.networks.EmailStrwill fail not when imported, but when it's used for typing check. Because insidepydantic.networks.EmailStr, there is a functionimport_email_validatorwhich will try to importemail_validatordinamically.Changes
In
aws_lambda_powertools/utilities/parser/models/ses.py:It means, now is checking for
email_validatorpackage, and defines conditionally which type it should takeUser experience
Use could perform:
without unexpected behavior such as:
ImportError: email-validator is not installed, run 'pip install pydantic[email]'Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.